txnprovider/txpool: wake a caller that goes away while waiting for a block - #23343
txnprovider/txpool: wake a caller that goes away while waiting for a block#23343lystopad wants to merge 2 commits into
Conversation
…block best parks in a sync.Cond, which has no notion of a context: the wait ends when a new block broadcasts the condition, or when the node shuts down. A caller that gave up in the meantime therefore stays parked, holding whatever it brought with it, until one of those happens. While the chain is stalled that is never, which is when it matters most. Cancelling now broadcasts, so the caller wakes and returns. The broadcast takes the pool lock, which is what keeps it from landing between the cancellation check and the wait, where a wakeup would be lost. Reachable once anything cancels a live build: discarding an evicted payload builder does, which is why this is a prerequisite for that change rather than part of it.
There was a problem hiding this comment.
The cancellation watcher is not joined before best returns. Closing waitDone only makes one select case ready; when ctx is already cancelled, the watcher may still choose the ctx branch and acquire p.lock after the call has returned. This makes TestBestReleasesTheLockWhenTheCallerGivesUpWaitingForABlock flaky at its TryLock assertion and means the watcher is not scoped to the call as described.
I reproduced it with:
GOMAXPROCS=4 go test -race ./txnprovider/txpool -run TestBestReleasesTheLockWhenTheCallerGivesUpWaitingForABlock -count=1000
Please synchronize watcher completion before returning, after any held pool lock has been released, or restructure the watcher so it cannot touch p.lock after best returns.
Closing its stop channel only made one of the watcher's two cases ready. With the context already ended it could pick the other, and take the pool lock after the call it belongs to had returned. Waiting for it to finish keeps it scoped to the call, as described. The wait is registered before the lock is taken, so it runs after the lock is released.
|
Fixed in You are right on both counts. Closing the stop channel only made one of the watcher's two cases ready, so with the context already ended it could pick the other and take the pool lock after the call had returned — which makes the flake real and makes "scoped to the call" untrue as I had described it. The watcher is joined now rather than merely told to stop. The join is registered before the lock is taken, so deferred order puts it after the lock is released and it cannot deadlock against the goroutine it is waiting for. Your reproduction passes: |
Second prerequisite for #23272, after #23333. Raised by @yperbasis reviewing that PR.
TxPool.bestparks inp.lastSeenCond.Wait()until the block it was asked to build on top of arrives.sync.Condhas no notion of a context: the wait ends when a new block broadcasts the condition (pool.go:361) or when the node shuts down. Cancelling the caller does nothing.So a caller that has given up stays parked, holding its read transaction and
SharedDomains, until a block arrives. While the chain is stalled that is never — which is exactly when builders accumulate and when releasing them matters.#23333 stopped a cancelled caller leaving the pool lock held. This is the other half: making the wait notice at all.
The fix
Cancellation broadcasts the condition, so the parked caller wakes, sees its context, and returns. Waiters re-check their own condition on waking regardless, so an extra broadcast is harmless.
The broadcast takes the pool lock. That is load-bearing rather than incidental: without it the broadcast could land between the cancellation check and
Wait(), and the wakeup would be lost — the caller would sleep on exactly as before. Holding the lock means the broadcast can only happen before the check, where the check sees it, or afterWait()has released the lock, where the broadcast reaches it.The watcher goroutine is scoped to the call and exits with it.
Test
TestBestReturnsWhenItsCallerGoesAwayWhileWaitingForABlockcancels only once the caller has reached the "Waiting for block" trace, so it exercises the wait rather than the check in front of it. Nothing else wakes it: no block arrives, nothing shuts down. Against the current code it fails withbest never returned; only a new block would have woken it.Why separate
It is a
txnprovider/txpoolchange, and #23272 is anexecution/change that is large already. Same reasoning as #23333, which @yperbasis suggested splitting out for the same reason.Note:
make lintcireportsdb/seg/decompress.go:199: field residencyOnce is unused, which is darwin-only — that field's user isresidency_gate_linux.go, so CI does not see it.golangci-lintis clean fortxnprovider/txpool/....